-
Notifications
You must be signed in to change notification settings - Fork 178
add processor chain and add support for model and tool #4093
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
db22e6f
to
49340dd
Compare
b6e9dbd
to
f648038
Compare
f648038
to
04b2fc5
Compare
04b2fc5
to
a196876
Compare
a196876
to
896b4d2
Compare
79f7649
to
e085a9b
Compare
modelTensor = ModelTensor.builder().name(outputKey).dataAsMap((Map) output).build(); | ||
} else if (output instanceof List) { | ||
Map<String, Object> resultMap = new HashMap<>(); | ||
resultMap.put("output", output); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we put the output in output
field here, but above we just return as is if it is a map, so output
key may or may not be there? is this expected
previousStepListener = nextStepListener; | ||
} | ||
} | ||
// firstTool.run(firstToolExecuteParams, firstStepListener); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
test code?
if (processedOutput instanceof String) { | ||
connector.parseResponse((String) processedOutput, modelTensors, scriptReturnModelTensor); | ||
} else { | ||
connector.parseResponse(processedOutput, modelTensors, scriptReturnModelTensor); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this condition seems wrong, if it is a string, you are casting to string else passing the object as is? then why not just pass it in the first place and check inside the parseResponse method
if (outputParser != null) { | ||
listener.onResponse((T) outputParser.parse(output)); | ||
} else { | ||
listener.onResponse((T) output); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
listener.onResponse((T) (outputParser != null ? outputParser.parse(output) : output));
return captures.get(0); | ||
} | ||
return captures; | ||
// return String.join(" ", captures); // join results with a space |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
test code?
} | ||
} | ||
if (captures.size() == 1) { | ||
return captures.get(0); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: use getFirst()
if (condition.startsWith(">") && !condition.startsWith(">=")) { | ||
double threshold = Double.parseDouble(condition.substring(1)); | ||
return numValue > threshold; | ||
} else if (condition.startsWith("<") && !condition.startsWith("<=")) { | ||
double threshold = Double.parseDouble(condition.substring(1)); | ||
return numValue < threshold; | ||
} else if (condition.startsWith(">=")) { | ||
double threshold = Double.parseDouble(condition.substring(2)); | ||
return numValue >= threshold; | ||
} else if (condition.startsWith("<=")) { | ||
double threshold = Double.parseDouble(condition.substring(2)); | ||
return numValue <= threshold; | ||
} else if (condition.startsWith("==")) { | ||
double threshold = Double.parseDouble(condition.substring(2)); | ||
return Math.abs(numValue - threshold) < 1e-10; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit (readability): we don't need else if as there is a return everywhere
rather:
if { return; }
if { return; }
if (replaceAll) { | ||
return p.matcher(text).replaceAll(replacement); | ||
} else { | ||
return p.matcher(text).replaceFirst(replacement); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: can just return, no need else
if () {return;} return;
if ("object".equalsIgnoreCase(extractType)) { | ||
if (jsonNode.isObject()) { | ||
return mapper.convertValue(jsonNode, Map.class); | ||
} else { | ||
return defaultValue != null ? defaultValue : input; | ||
} | ||
} else if ("array".equalsIgnoreCase(extractType)) { | ||
if (jsonNode.isArray()) { | ||
return mapper.convertValue(jsonNode, List.class); | ||
} else { | ||
return defaultValue != null ? defaultValue : input; | ||
} | ||
} else { // auto | ||
if (jsonNode.isObject()) { | ||
return mapper.convertValue(jsonNode, Map.class); | ||
} else if (jsonNode.isArray()) { | ||
return mapper.convertValue(jsonNode, List.class); | ||
} else { | ||
return defaultValue != null ? defaultValue : input; | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same here, let's use early return, really helps with readability, too many if/elseif will cause issues
Overall looks good, left comments to clean up code. Will test it out more thoroughly. |
e085a9b
to
a68f8ef
Compare
try { | ||
return Pattern.matches(regex, strValue); | ||
} catch (Exception e) { | ||
log.warn("Invalid regex in condition: {}", regex); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we throw an error here instead of failing silently?
} | ||
``` | ||
|
||
Sampel output |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: typo
@@ -0,0 +1,543 @@ | |||
# 1. Create Model |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we explain what the tutorial is for before jumping into steps?
@@ -0,0 +1,633 @@ | |||
# 1. Create Model |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same feedback, could we explain what this tutorial does?
{ | ||
"name": "Query DSL Translator Agent", | ||
"type": "flow", | ||
"description": "This is a demo agent for translating NLQ to OpenSearcdh DSL", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: typo
} | ||
``` | ||
|
||
Sampel output |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: typo
{ | ||
"name": "Query DSL Translator Agent", | ||
"type": "flow", | ||
"description": "This is a demo agent for translating NLQ to OpenSearcdh DSL", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: typo
Overall LGTM, have some smaller comments. |
@ylwu-amzn let's separate the tutorial and the PR? Can you raise a separate PR with the tutorial changes? |
Signed-off-by: Yaliang Wu <[email protected]>
Signed-off-by: Yaliang Wu <[email protected]>
Signed-off-by: Yaliang Wu <[email protected]>
Signed-off-by: Yaliang Wu <[email protected]>
Signed-off-by: Yaliang Wu <[email protected]>
a68f8ef
to
583df1a
Compare
Description
When build tutorial for Agentic RAG with Bedrock OpenAI OSS model, see the converse API returns different result with Bedrock Claude model.
Build post process function with Painless Script is painful. Propose a new processor framework for processing outputs from ML models and tools using chainable processors
Supports multiple built-in processors:
to_string
: Converts input to string formatregex_replace
: Performs regex-based text replacementjsonpath_filter
: Filters JSON using JsonPath expressionsextract_json
: Extracts JSON objects/arrays from textregex_capture
: Captures text using regex groupsremove_jsonpath
: Removes elements at specified JsonPathconditional
: Applies different processors based on conditionsTest Example
Create Agent with ListIndexTool.
Configure two output processors for ListIndexTool
Sample resplonse
Related Issues
Resolves #[Issue number to be closed when this PR is merged]
Check List
--signoff
.By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.